Store container manifest #1830
Store container manifest #1830
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance improvement for bootc status by storing the container manifest and config locally in an .imginfo file, avoiding slow network requests. The implementation also refactors the code to use ManifestDiff::new for a cleaner diffing logic.
The changes are well-structured and achieve the intended goal. However, I've found a couple of critical issues related to memory safety and type mismatches that need to be addressed. I've also included a suggestion to improve backward compatibility for older systems.
Overall, this is a great enhancement. Once the identified issues are resolved, this will be a solid contribution.
|
|
||
| (digest, version, arch, created) | ||
| } | ||
| let img_conf = get_imginfo(storage, &verity)?; |
There was a problem hiding this comment.
This change removes the fallback mechanism that was present before. If get_imginfo fails (e.g., for an older deployment without an .imginfo file), this will cause bootc status and other commands to fail. While the PR description mentions breaking older systems, a status command should ideally be more resilient.
Consider adding a fallback to the previous behavior of fetching the manifest from the registry if the local .imginfo file is not available. This would provide backward compatibility and a more robust user experience.
There was a problem hiding this comment.
Consider adding a fallback to the previous behavior of fetching the manifest from the registry if the local .imginfo file is not available.
Yeah I also lean towards keeping the previous code for now if it's not too hard
There was a problem hiding this comment.
It's definitely not difficult, but we did not want to make a network request on every bootc status so I left it out. Guess we won't make the request anyway after this change. But, also, the previous code had a bug where it would show the "latest" shasum of the image on bootc status even when the image was not pulled into the composefs repo, because we were just fetching everything from a remote repository
There was a problem hiding this comment.
Made some changes to be backwards compatible. We will just default to our older method of fetching info from registry if the .imginfo file doesn't exist, then populate the .imginfo file
Johan-Liebert1
force-pushed
the
container-manifest
branch
from
e04174f to
34e563e
Compare
Until now, when doing a `bootc status` for a compoesfs booted system, we were reaching out a container registry to fetch image manifest and config, which is pretty suboptimal as the command took upwards of 1.5s to execute, sometimes. Instead, now we store the manifest + config as a JSON structure inside an `.imginfo` file alongside the `.origin` file Signed-off-by: Pragyan Poudyal <[email protected]>
Now that we store the current deployment's manifest locally, we can replace the ugly stuff with a simple call to `ManifestDiff::new` Signed-off-by: Pragyan Poudyal <[email protected]>
Make `container_details` argument mandatory while writing composefs deployment state. It's better to fetch the container details from the source imgref, rather than the target imgref, as the target might not even exist at the time of deployment. Fixes CI failures as we were fetching from local registry (according to the target imgref), which doesn't exist Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
container-manifest
branch
from
13bef32 to
b0ad0a7
Compare
For older systems that do not have the `.imginfo` file, we revert back to our older logic of fetching the image info from registry We then store this in an `.imginfo` file so we don't go and make a network call on every subsequent command Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
container-manifest
branch
from
b0ad0a7 to
f6f46d2
Compare
|
This one relates to #1601 - if we got to the point of doing that by default, then we could always fetch metadata from the containers-storage instance. |
2 participants
Until now, when doing a
bootc statusfor a compoesfs booted system, wewere reaching out a container registry to fetch image manifest and
config, which is pretty suboptimal as the command took upwards of 1.5s
to execute, sometimes.
Instead, now we store the manifest + config as a JSON structure inside
an
.imginfofile alongside the.originfileNow that we store the current deployment's manifest locally, we can
replace the ugly stuff with a simple call to
ManifestDiff::new